Skip to content

Name the writing release in the storage-format upgrade refusal#315

Merged
aaltshuler merged 1 commit into
mainfrom
audit-versioning-practices
Jul 6, 2026
Merged

Name the writing release in the storage-format upgrade refusal#315
aaltshuler merged 1 commit into
mainfrom
audit-versioning-practices

Conversation

@ragnorc

@ragnorc ragnorc commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

What & why

The storage-format upgrade refusal told operators to export with "the older omnigraph binary that created it" without saying which release — leaving them to guess, and stuck if the fleet was already upgraded. This names the exact release line that wrote each internal-schema stamp (e.g. v3 → 0.6.2–0.7.2) plus the precise export/init/load commands, keeping the strand-model upgrade fail-closed but self-service. The message is engine-shared, so server boot-quarantine logs and cluster status observations get the better text for free.

Backing issue / RFC

  • Trivial fast-lane — a focused UX/docs improvement to the already-documented export/import upgrade path; no architecture change and the strand model is untouched. (Maintainer internal process applies.)

Checklist

  • Change is focused (one logical change)
  • Tests added/updated for behavior changes — in-source refusal assertion (red→green) + a gated genuine-v3 cross-version round-trip test
  • Public docs updated — upgrade.md (release-named message + stamp→release table), versioning.md, testing.md
  • Reviewed against docs/dev/invariants.md — no Hard Invariant weakened; the refusal is still fail-closed in both directions; in-engine read of old formats was deliberately NOT added

Notes for reviewers

  • Stamp→release map is derived from the release tags (verified: v3 starts at 0.6.2, not 0.7.0).
  • crates/omnigraph-cli/tests/crossversion_upgrade.rs mints a real v3 graph with omnigraph 0.7.2 and is gated on OMNIGRAPH_OLD_BIN; it skips in the default suite and runs in the new gated crossversion_upgrade CI job (installs omnigraph-cli@0.7.2). It is the empirical proof the existing stamp-rewind test can't give.

Note

Low Risk
Messaging and documentation only on an already fail-closed open path; no change to migration policy or in-place upgrades. Residual risk is limited to incorrect stamp→release mapping in operator-facing text.

Overview
When a graph’s internal-schema stamp is below what the current binary serves, the refusal message is more actionable: it names the release line that wrote that stamp (via new release_for_internal_schema_version in migrations.rs) and spells out the full exportinitload rebuild steps instead of telling operators to use “the older binary” with no version hint. Open logic and the strict single-version strand model are unchanged—still fail-closed.

Tests and CI: In-source checks assert the named release appears in refusals; crossversion_upgrade.rs (gated on OMNIGRAPH_OLD_BIN) builds a genuine v3 graph with omnigraph 0.7.2 and verifies refusal text plus export round-trip including vectors. A new crossversion_upgrade job and run_upgrade_ci path filter run that test on PRs that touch the upgrade surface.

Docs: upgrade.md, versioning.md, and testing.md document the new message, stamp→release table, and how to run the gated test locally and in CI.

Reviewed by Cursor Bugbot for commit e79b1cb. Bugbot is set up for automated code reviews on this repo. Configure here.

Greptile Summary

This PR improves the operator experience for the storage-format upgrade path by making the sub-CURRENT refusal message self-service: it now names the exact release line that wrote each internal-schema stamp (via release_for_internal_schema_version in migrations.rs) and prints the full exportinitload command sequence. No behavioral change to the strand model or open-path logic.

  • Core code change (migrations.rs): a new release_for_internal_schema_version(stamp) function maps v1–v4 stamps to release strings; refuse_if_stamp_unsupported embeds this in the sub-CURRENT error so operators know which binary to fetch.
  • Test coverage: tests.rs asserts the named release appears in the refusal; crossversion_upgrade.rs (gated on OMNIGRAPH_OLD_BIN) mints a genuine v3 graph with the 0.7.2 binary and verifies the refusal text plus a full round-trip including a Vector column.
  • Docs update: upgrade.md adds the stamp→release table (now covering v1–v4), reproduces the updated error message verbatim, and documents versioning.md and testing.md to reflect the new function and CI job.

Confidence Score: 5/5

Safe to merge — the change is limited to the text of an existing error message and its surrounding documentation; the refusal logic itself is untouched and still fail-closed.

The only code path changed is the string returned by the new release_for_internal_schema_version function and its embedding in the error message. The open-path guard, stamp comparison, and MIN_SUPPORTED == CURRENT invariant are all unchanged. Tests cover the new assertion both in-source (synthetic stamp rewind) and via a real cross-version binary.

The 4 => "0.8.x" arm in migrations.rs differs from the "0.8.x and later" wording in upgrade.md — a minor drift that only matters once a future release maintains v4 without updating the string.

Important Files Changed

Filename Overview
crates/omnigraph/src/db/manifest/migrations.rs Adds release_for_internal_schema_version mapping stamps to release strings; updates refuse_if_stamp_unsupported to embed the release name in the error. The _ fallback is reworded to "an unrecognized older release" (addressing prior review) and is unreachable in practice. The v4 entry maps to "0.8.x" — consistent with the current binary but slightly narrower than the docs' "0.8.x and later" phrasing.
crates/omnigraph/src/db/manifest/tests.rs Adds release_names_the_writing_line_for_each_stamp (unit) and extends sub_current_graph_is_refused_then_rebuilt_via_export_import to assert both "0.7.2" and "export" appear in the refusal message. The new assertions are well-targeted and match the exact message template.
crates/omnigraph-cli/tests/crossversion_upgrade.rs New gated integration test that mints a real v3 graph via OMNIGRAPH_OLD_BIN, asserts the current binary refuses it with the full range string "0.6.2 to 0.7.2", then exercises the full export→init→load round-trip including a Vector column. The skip-on-unset pattern matches the S3 gate convention and the assertion is now as tight as the in-source twin.
docs/user/operations/upgrade.md Adds the v4 row to the stamp→release table and shows the named error message verbatim. The table row says "0.8.x and later" while migrations.rs maps 4 => "0.8.x" — a minor inconsistency that becomes actionable only if a future release maintains v4 without updating the code string.
docs/dev/versioning.md References release_for_internal_schema_version by name and documents the fail-closed + self-service property of the new refusal message; no structural changes to the versioning policy.
docs/dev/testing.md Documents crossversion_upgrade.rs in the CLI test surface table and adds the "Cross-version upgrade" section explaining the OMNIGRAPH_OLD_BIN gate; consistent with the new CI job.
docs/rfcs/rfc-0015-ingest-embeddings.md Minor documentation update; no functional concerns.
docs/rfcs/rfc-0018-ingest-wal.md Minor documentation update; no functional concerns.
docs/rfcs/rfc-0019-heads-and-fences.md Minor documentation update; no functional concerns.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Omnigraph::open / open_read_only] --> B[read_stamp from __manifest]
    B --> C{stamp vs CURRENT}
    C -->|stamp > CURRENT| D["Error: upgrade omnigraph\nbefore opening this graph"]
    C -->|stamp == CURRENT| E[Open succeeds]
    C -->|stamp < MIN_SUPPORTED| F[release_for_internal_schema_version stamp]
    F --> G{Match stamp}
    G -->|1| H["0.3.1 or earlier"]
    G -->|2| I["0.4.1 to 0.6.1"]
    G -->|3| J["0.6.2 to 0.7.2"]
    G -->|4| K["0.8.x"]
    G -->|_| L["an unrecognized older release"]
    H & I & J & K & L --> M["Error: __manifest stamped at v{stamp}...\nThis graph was created by omnigraph {release}.\nRebuild: export → init → load"]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Omnigraph::open / open_read_only] --> B[read_stamp from __manifest]
    B --> C{stamp vs CURRENT}
    C -->|stamp > CURRENT| D["Error: upgrade omnigraph\nbefore opening this graph"]
    C -->|stamp == CURRENT| E[Open succeeds]
    C -->|stamp < MIN_SUPPORTED| F[release_for_internal_schema_version stamp]
    F --> G{Match stamp}
    G -->|1| H["0.3.1 or earlier"]
    G -->|2| I["0.4.1 to 0.6.1"]
    G -->|3| J["0.6.2 to 0.7.2"]
    G -->|4| K["0.8.x"]
    G -->|_| L["an unrecognized older release"]
    H & I & J & K & L --> M["Error: __manifest stamped at v{stamp}...\nThis graph was created by omnigraph {release}.\nRebuild: export → init → load"]
Loading

Reviews (6): Last reviewed commit: "docs: self-service upgrade message + gat..." | Re-trigger Greptile

Comment thread docs/user/operations/upgrade.md
Comment thread crates/omnigraph-cli/tests/crossversion_upgrade.rs
Comment thread crates/omnigraph/src/db/manifest/migrations.rs Outdated
Comment thread .github/workflows/ci.yml Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 84449856f0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/omnigraph-cli/tests/crossversion_upgrade.rs Outdated
ragnorc added a commit that referenced this pull request Jun 30, 2026
Address P2 review feedback on #315:

- crossversion_upgrade.rs: OMNIGRAPH_OLD_BIN set-but-missing now fails loudly
  instead of skipping vacuously (only an UNSET var is a legitimate skip) — a
  misconfigured CI install path no longer passes without minting a v3 graph.
- crossversion_upgrade.rs: tighten the refusal assertion from `0.7` to the full
  `0.6.2 to 0.7.2` range, matching its in-source sibling.
- ci.yml: add db/manifest/tests.rs to the run_upgrade_ci path filter so a future
  edit to the in-source refusal assertion still triggers the cross-version job.
- migrations.rs: reword the unreachable release fallback to read naturally after
  "created by omnigraph " (defensive only; 1-3 are mapped, >=CURRENT is caught
  by the ceiling guard first).

Declined: adding a v4 row to the upgrade.md stamp->release table — that table is
for sub-CURRENT (refused) stamps only, and a v4 graph under a v4 binary is never
refused, so a v4 row would wrongly imply a current graph needs rebuilding.
@ragnorc

ragnorc commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review — addressed in 817979e1:

  • old_bin() vacuous skip (Codex): a set-but-missing OMNIGRAPH_OLD_BIN now fails loudly instead of skipping; only an unset var is a legitimate skip, so a misconfigured CI install path can't pass without minting a v3 graph.
  • Loose cross-version assertion (Greptile): tightened "0.7" → the full "0.6.2 to 0.7.2" range, matching its in-source sibling.
  • tests.rs not in the CI filter (Greptile): added crates/omnigraph/src/db/manifest/tests.rs to run_upgrade_ci so a future edit to the in-source refusal assertion still triggers the cross-version job.
  • Awkward fallback grammar (Greptile): reworded the (unreachable) _ arm to read naturally after "created by omnigraph ".

Declined: the v4 row in the upgrade.md stamp→release table. That table answers "which old binary do I need?" for sub-CURRENT stamps that get refused. A v4 graph under a v4 binary is never refused — it opens normally — so a v4 row would wrongly imply a current graph needs rebuilding. v4 joins the table when v5 ships and the doc is revised.

@aaltshuler

Copy link
Copy Markdown
Collaborator

Rebased onto current main (post Lance 9.0.0-beta.15) as a clean re-apply — the branch's merge-commit history made a plain rebase impractical, so this is the PR's net diff on a fresh base with authorship preserved. Conflicts were testing.md row noise; one substantive touch-up: the CI install comment claimed 0.7.2 shares Lance 7.0.0 with current, which the Lance 9 migration made stale — reworded, and notably the cross-version test now doubles as a cross-substrate check (Lance-9 binary reading a Lance-7-written manifest to find the stamp). Validated locally against a genuine crates.io 0.7.2 binary: the v3 mint → refusal-with-named-release → rebuild round-trip passes. Full workspace gate green.

@aaltshuler aaltshuler force-pushed the audit-versioning-practices branch from e79b1cb to 99e49f8 Compare July 6, 2026 01:14
@aaltshuler

Copy link
Copy Markdown
Collaborator

Per review discussion: the CI job is removed entirely — ci.yml is now byte-identical to main. The cross-version test stays, gated on OMNIGRAPH_OLD_BIN (skips gracefully in the default gate), with the on-demand invocation documented in testing.md under the same disposition as write_cost_s3: compiling 0.7.2 from source is a 15–25 min job and the format-stamp seam changes a few times a year, so it runs when touching migrations.rs/loader/export or at release boundaries, not per-PR. Validated locally against a genuine 0.7.2 binary on the Lance-9 main.

@aaltshuler aaltshuler force-pushed the audit-versioning-practices branch from 99e49f8 to c6a3b2e Compare July 6, 2026 01:29
…and)

The storage-format refusal now names the release line that wrote each
internal-schema stamp (e.g. v3 -> 0.6.2-0.7.2) plus the exact
export/init/load commands — fail-closed but self-service; the message is
engine-shared so server boot-quarantine and cluster status get it free.

Adds the OMNIGRAPH_OLD_BIN-gated cross-version test: mint a GENUINE v3
graph with a real omnigraph-cli 0.7.2 binary, assert the current binary
refuses it with the release-named message, and the documented rebuild
round-trips. Deliberately NOT wired into CI (the write_cost_s3
disposition): compiling 0.7.2 from source is a 15-25 min job and the
stamp seam changes a few times a year — testing.md documents the
on-demand invocation instead.

Rebased onto current main across the Lance 9.0.0-beta.15 migration;
validated locally against a genuine 0.7.2 binary — the v3 round-trip
passes on the Lance-9 main, which also makes it a live cross-substrate
read-compat check (a Lance-9 binary reading a Lance-7-written manifest
to find the stamp).
@aaltshuler aaltshuler force-pushed the audit-versioning-practices branch from c6a3b2e to e59fea8 Compare July 6, 2026 01:31
@aaltshuler aaltshuler merged commit ca2ad97 into main Jul 6, 2026
7 checks passed
@aaltshuler aaltshuler deleted the audit-versioning-practices branch July 6, 2026 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants